feat(studio): Additional numerical range filters - #577
Conversation
…rs.<name>.mean Signed-off-by: Nicholas Kolean <nakolean@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends ChangesDynamic filterFieldMap and evaluator column filtering
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ExperimentGroupDataView
participant useStudioDataViewState
participant useCustomReactTable
participant backend API
ExperimentGroupDataView->>useStudioDataViewState: filterFieldMap(id)
useStudioDataViewState->>useCustomReactTable: table filter state
useCustomReactTable->>backend API: apiFilter.filter with mapped keys
backend API-->>ExperimentGroupDataView: filtered results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/packages/common/src/hooks/useStudioDataViewState/index.ts (1)
484-503: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valuePossible silent filter collision via
Object.fromEntries.If
filterFieldMap(function or record form) maps two distinct filter ids to the same key,Object.fromEntriessilently drops the earlier entry — one active UI filter would be dropped without any warning. Not exploited by current callers (getExperimentFilterFieldmaps distinct ids to distinct keys), but nothing here guards against it as the mapping function grows.🛡️ Optional dedup guard
- result.filter = Object.fromEntries( - debouncedColumnFilters - .filter((f) => { ... }) - .map((f) => { - const mappedKey = - typeof filterFieldMap === 'function' ? filterFieldMap(f.id) : filterFieldMap?.[f.id]; - return [mappedKey ?? f.id, f.value]; - }) - ) as Partial<FilterType>; + const entries = debouncedColumnFilters + .filter((f) => { ... }) + .map((f) => { + const mappedKey = + typeof filterFieldMap === 'function' ? filterFieldMap(f.id) : filterFieldMap?.[f.id]; + return [mappedKey ?? f.id, f.value] as const; + }); + if (process.env.NODE_ENV !== 'production') { + const seen = new Set<string>(); + for (const [key] of entries) { + if (seen.has(key)) console.warn(`apiFilter: duplicate mapped key "${key}" — a filter was dropped`); + seen.add(key); + } + } + result.filter = Object.fromEntries(entries) as Partial<FilterType>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/hooks/useStudioDataViewState/index.ts` around lines 484 - 503, The filter assembly in useStudioDataViewState is vulnerable to silent collisions when filterFieldMap returns the same mapped key for multiple debouncedColumnFilters entries, because Object.fromEntries will overwrite earlier values. Update the filter-building logic around debouncedColumnFilters to detect duplicate mapped keys before constructing result.filter, and either skip duplicates deterministically or surface a clear warning/error when filterFieldMap (function or record form) maps multiple ids to the same key.web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx (1)
48-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
evaluator-<name>regex — extract a shared helper.The same
/^evaluator-(.+)$/pattern is duplicated inderiveEvaluatorNames(util.ts) and here. Two independent copies of the evaluator-id convention risk silent drift if the naming scheme ever changes.Proposed refactor
+// util.ts +export const parseEvaluatorId = (id: string): string | undefined => + id.match(/^evaluator-(.+)$/)?.[1];- const evaluatorMatch = id.match(/^evaluator-(.+)$/); - if (evaluatorMatch) return `evaluators.${evaluatorMatch[1]}.mean`; + const evaluatorName = parseEvaluatorId(id); + if (evaluatorName) return `evaluators.${evaluatorName}.mean`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx` around lines 48 - 56, The evaluator-id regex is duplicated between getExperimentFilterField and deriveEvaluatorNames, so extract the /^evaluator-(.+)$/ parsing into a shared helper and reuse it from both places. Update the helper to return the evaluator name/field mapping once, then have ExperimentGroupDataView and util.ts call that shared function instead of maintaining separate copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/packages/common/src/hooks/useStudioDataViewState/index.ts`:
- Around line 484-503: The filter assembly in useStudioDataViewState is
vulnerable to silent collisions when filterFieldMap returns the same mapped key
for multiple debouncedColumnFilters entries, because Object.fromEntries will
overwrite earlier values. Update the filter-building logic around
debouncedColumnFilters to detect duplicate mapped keys before constructing
result.filter, and either skip duplicates deterministically or surface a clear
warning/error when filterFieldMap (function or record form) maps multiple ids to
the same key.
In
`@web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx`:
- Around line 48-56: The evaluator-id regex is duplicated between
getExperimentFilterField and deriveEvaluatorNames, so extract the
/^evaluator-(.+)$/ parsing into a shared helper and reuse it from both places.
Update the helper to return the evaluator name/field mapping once, then have
ExperimentGroupDataView and util.ts call that shared function instead of
maintaining separate copies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d08a96d-b7c2-4160-8f56-83b7943b4e74
📒 Files selected for processing (6)
web/packages/common/src/hooks/useStudioDataViewState/filterFieldMap.integration.test.tsxweb/packages/common/src/hooks/useStudioDataViewState/index.test.tsxweb/packages/common/src/hooks/useStudioDataViewState/index.tsweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/util.test.tsweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/util.ts
|
shanaiabuggy
left a comment
There was a problem hiding this comment.
Beautiful! Can we include Run Count in the set of filters as well?
Signed-off-by: Nicholas Kolean <nakolean@gmail.com>
* feat(studio): Additional numerical range filter for cost and evaluators.<name>.mean Signed-off-by: Nicholas Kolean <nakolean@gmail.com> * include run count Signed-off-by: Nicholas Kolean <nakolean@gmail.com> --------- Signed-off-by: Nicholas Kolean <nakolean@gmail.com>
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Tests